Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Increase timeout for process termination #1422

Open
wants to merge 1 commit into
base: nightly
Choose a base branch
from

Conversation

GiantTreeLP
Copy link
Member

Motivation

Current Java Edition Minecraft servers, especially those that are forks of the vanilla Minecraft server software, take some time to properly stop and save all resources, i.e. worlds, inventories and other state to disk. Prematurely terminating those processes leads to worlds not being saved at all, world corruption due to the interrupted save process and unloadable worlds. In addition this leads to lock files being left that don't belong to any process anymore.

Modification

Increase the timeout to 60 seconds. This gives servers more time to properly store all files and shutdown whilst still cleaning up servers that may have become stuck whilst shutting down.

Result

Server processes are now given more time to properly shutdown.

Other context

See the discussion in Discord starting from here.

### Motivation
Current Java Edition Minecraft servers, especially those that are forks of the vanilla Minecraft server software, take some time to properly stop and save all resources, i.e. worlds, inventories and other state to disk.
Prematurely terminating those processes leads to worlds not being saved at all, world corruption due to the interrupted save process and unloadable worlds. In addition this leads to lock files being left that don't belong to any process anymore.

### Modification
Increase the timeout to 60 seconds. This gives servers more time to properly store all files and shutdown whilst still cleaning up servers that may have become stuck whilst shutting down.

### Result
Server processes are now given more time to properly shutdown.
@DasBabyPixel
Copy link
Contributor

I feel like this should be configurable instead of fixed 60s (or 5s)
Best would be a setting for the task imo. Some lobby template servers only need 5s and should be killed after, some heavily modded static server might need more. Yet you also don't want to wait for a whole minute for broken lobby server to be killed.

@derklaro
Copy link
Member

Yes, the configuration should be available per-service, but that's not a simple task to do as the task configuration is not available to the service, unless we provide the timeout as a variable in the service configuration or retrieve the task from the task name provided to the service configuration.

@DasBabyPixel
Copy link
Contributor

Yes, the configuration should be available per-service, but that's not a simple task to do as the task configuration is not available to the service, unless we provide the timeout as a variable in the service configuration or retrieve the task from the task name provided to the service configuration.

what's the problem with providing it as a variable in the service configuration?

@derklaro
Copy link
Member

The thing is: sure, we could just throw the variable in there and move on, but there are more places that have variables that cannot be set per service, and need to be accessed via the task (which shouldn't be the case). One example is the required permission for a task or the smart configuration. So imo it's better to delay this a bit and find a better solution on how we can handle the variables and then merge this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants